AI Code Review

🔶 Changes requested — one or more medium-severity items must be addressed

What was reviewed

Skipped (out of scope): dist/, pnpm-lock.yaml

Findings

🔶 Medium — excelDate type signature allows {t:"d", v: string} — invalid SheetJS cell

File: packages/pluggableWidgets/datagrid-web/src/features/data-export/cell-readers.ts line 71
Problem: excelDate accepts value: string | Date and format?: string. When a caller passes a string value with a non-undefined format, the function returns { t: "d", v: "<string>" } — an invalid SheetJS cell (SheetJS requires t:"d" cells to have a Date as v). The current callers happen not to hit this combination, but the type signature doesn't prevent it. A future caller could easily produce a corrupt export.
Fix: Overload or split the function so t:"d" is only produced when v is a Date:

export function excelDate(value: string): ExcelCell;
export function excelDate(value: Date, format: string): ExcelCell;
export function excelDate(value: string | Date, format?: string): ExcelCell {
    return {
        t: format === undefined || typeof value === "string" ? "s" : "d",
        v: value,
        z: format
    };
}

🔶 Medium — dynamicText reader computes format but never uses it for numeric/date coercion

File: packages/pluggableWidgets/datagrid-web/src/features/data-export/cell-readers.ts line 142
Problem: The dynamicText reader calls getCellFormat(...) and passes format to excelString. However, excelString only uses format to set the z field — there is no coercion to t:"n" or t:"d" regardless of exportType. This means a dynamicText column with exportType: "number" still exports as a string cell, which is the bug described in problem #1 of the PR for customContent. The same silent no-op exists here but is untested and undocumented.
Fix: Either apply the same number/date branching as customContent, or explicitly document and test that dynamicText always exports as string (and why). If the intent is string-only, remove the getCellFormat call to avoid confusion.

⚠️ Low — hasTimeComponent regex misses minute-only formats like "mm:ss"

File: packages/pluggableWidgets/datagrid-web/src/features/data-export/cell-readers.ts line 86
Note: /[hs]/i detects h (hours) and s (seconds). A format like "mm:ss" (minutes and seconds, no hours) would match s and correctly preserve time — that path is fine. However, a format of "HH:mm" (hours and minutes, locale variants) would also match H via the case-insensitive flag — also correct. The one edge case not covered by a test is "ss" alone (elapsed seconds format) — worth adding a test case for documentation. No code change required if the current behavior is intentional.

⚠️ Low — excelString has redundant ?? undefined

File: packages/pluggableWidgets/datagrid-web/src/features/data-export/cell-readers.ts line 67
Note: z: format ?? undefined — since format is already string | undefined, this is a no-op. Simplify to z: format.

⚠️ Low — Plain number type path in attribute reader is untested

File: packages/pluggableWidgets/datagrid-web/src/features/data-export/__tests__/cell-readers.spec.ts
Note: The attribute reader at line 131 has a branch for typeof value === "number" (native JS number, not Big). All tests use new Big(...). Since Mendix's ListAttributeValue for numeric types always returns Big, this branch is likely dead code — if so, remove it. If it can be reached, add a test with a plain number builder or a comment explaining when it applies.

Positives

• Clean extraction into a pure-function module (cell-readers.ts) with zero side effects — easy to test and reason about.
• The countSignificantDigits guard for Big precision handles leading zeros, decimals, and negative values correctly.
• stripTime uses Date.UTC consistently, avoiding local-timezone contamination in exported date values.
• Boolean fix (Yes/No string cell instead of {t:"b"}) matches grid display and avoids the Excel TRUE/FALSE rendering issue cleanly.
• Test structure uses the column() factory helper from test-utils correctly, keeping tests concise and avoiding repetition.
• CHANGELOG entries are clear, user-facing, and cover all four fixed bugs.

AI Code Review

⚠️ Approved with suggestions — low-severity items only, safe to merge

What was reviewed

Skipped (out of scope): dist/, pnpm-lock.yaml

Findings

⚠️ Low — hasTimeComponent regex misses minutes token m

File: packages/pluggableWidgets/datagrid-web/src/features/data-export/cell-readers.ts line 89
Note: The regex /[hs]/i matches h (hours) and s (seconds) but not m (minutes). A format like "hh:mm" would correctly match on h, but a pure "mm:ss" (minutes + seconds without an hour component) would not match h — it would still match s, so that edge case is covered. However, a format like "HH:MM" (uppercase) in a custom SheetJS format where only M = minutes is used would be missed entirely. In practice, any real time format includes h/H, so this is unlikely to cause a real regression, but the regex is under-documented.
Fix: Add a comment explaining which tokens are checked, or broaden to /[hms]/i to be safe:

// h = hours, m = minutes, s = seconds
function hasTimeComponent(format: string): boolean {
    return /[hms]/i.test(format);
}

⚠️ Low — exportValue read without checking .status

File: packages/pluggableWidgets/datagrid-web/src/features/data-export/cell-readers.ts line 157
Note: props.exportValue?.get(item).value ?? "" accesses .value without first checking .status === "available". If the value is in "loading" state, .value may be undefined (hence the ?? ""), but this silently emits an empty cell rather than signalling the loading state. The attribute and dynamicText readers both gate on .status; customContent does not. This was the same in the original code, so it's not a regression introduced here, but since the module is being rewritten it's a good opportunity to align.
Fix:

customContent(item, props) {
    const raw = props.exportValue?.get(item);
    if (!raw || raw.status !== "available") {
        return makeEmptyCell();
    }
    const value = raw.value ?? "";
    // ... rest of reader
}

⚠️ Low — excelDate overloads allow Date without format to slip through at runtime

File: packages/pluggableWidgets/datagrid-web/src/features/data-export/cell-readers.ts lines 71–79
Note: The two overload signatures enforce that Date requires a format argument at compile time, but the runtime implementation t: value instanceof Date && format !== undefined ? "d" : "s" silently falls back to "s" if a Date is passed without a format (which TypeScript should prevent, but any-typed call sites bypass this). The original code had the same shape; no regression introduced here, but since this is a clean extraction it's worth tightening.
Fix: Add a runtime guard or a non-null assertion with a comment, to make the intent explicit and catch unexpected call sites:

export function excelDate(value: string | Date, format?: string): ExcelCell {
    if (value instanceof Date && format === undefined) {
        // Should not happen — TypeScript overloads require format for Date
        return makeEmptyCell();
    }
    return {
        t: value instanceof Date ? "d" : "s",
        v: value,
        z: format
    };
}

Positives

• Clean separation of concerns: extracting all cell-creation logic into a pure-function module (cell-readers.ts) with zero side effects makes the code straightforward to test and reason about in isolation.
• Test coverage is thorough — all four reader types, edge cases (NaN, empty strings, parse failures), and the new time-stripping logic are all explicitly exercised using @mendix/widget-plugin-test-utils builders rather than manual mock objects.
• The countSignificantDigits approach using Big.toFixed() + string manipulation is correct and avoids floating-point representation issues that value.toNumber() would introduce for big integers.
• CHANGELOG entries are clear, user-facing, and follow the Keep a Changelog format already established in the file.

AI Code Review

🔶 Changes requested — one or more medium-severity items must be addressed

What was reviewed

Skipped (out of scope): dist/, pnpm-lock.yaml

Findings

🔶 Medium — Whitespace-only strings silently export as the number 0

File: packages/pluggableWidgets/datagrid-web/src/features/data-export/cell-readers.ts line 164–167
Problem: Number(" ") evaluates to 0, not NaN. So when a user's exportValue expression returns a whitespace-only string and exportType is "number", the guard value !== "" passes, !Number.isNaN(0) is true, and the cell is exported as the number 0 — silently discarding the actual content. This differs from the intent of the fallback documented in the PR description.
Fix:

if (props.exportType === "number" && value.trim() !== "") {
    const parsed = Number(value);
    if (!Number.isNaN(parsed)) {
        return excelNumber(parsed, format);
    }
}

A companion test:

it("falls back to string for whitespace-only value with number exportType", () => {
    const col = column("Ws", c => {
        c.showContentAs = "customContent";
        c.exportValue = listExpression(() => "   ");
        c.exportType = "number";
    });
    const cell = readSingleCell(col);
    expect(cell.t).toBe("s");
    expect(cell.v).toBe("   ");
});

⚠️ Low — hasTimeComponent regex matches the S in locale specifiers

File: packages/pluggableWidgets/datagrid-web/src/features/data-export/cell-readers.ts line 89
Note: /[hs]/i (case-insensitive) matches any h, H, s, or S in the format string. A locale-tagged format like "[$-en-US]mmmm d, yyyy" contains a capital S in US, so hasTimeComponent incorrectly returns true and the time is not stripped — even though the format is date-only. Locale-tagged formats are uncommon in this context, but worth hardening. One approach: strip bracket-delimited locale tokens before testing, or use a stricter token regex:

function hasTimeComponent(format: string): boolean {
    // Strip locale tags like [$-en-US] before checking for time tokens
    const stripped = format.replace(/\[.*?\]/g, "");
    return /[hs]/i.test(stripped);
}

⚠️ Low — typeof value === "number" branch was removed without a comment

File: packages/pluggableWidgets/datagrid-web/src/features/data-export/cell-readers.ts line 133
Note: The old attribute reader handled both value instanceof Big and typeof value === "number". The new code only handles Big. If a Mendix attribute ever surfaces a plain JS number (e.g., an integer constant from a calculated attribute), it falls through to excelString(data.displayValue ?? "") — exporting a number as text without any warning. If this branch was intentionally dropped because Mendix guarantees numeric attributes are always Big, add a brief comment or an exhaustive check to make that contract explicit.

⚠️ Low — Missing test for dynamicText reader unavailable path

File: packages/pluggableWidgets/datagrid-web/src/features/data-export/__tests__/cell-readers.spec.ts
Note: The dynamicText reader returns excelString("n/a") when data.status === "unavailable" (line 150 of cell-readers.ts), but there is no test covering this branch. Since "n/a" is a meaningful user-visible fallback, add a test:

it("returns n/a cell when dynamicText is unavailable", () => {
    const col = column("Label", c => {
        c.showContentAs = "dynamicText";
        c.dynamicText = listExpression(() => "text", "unavailable");  // or however the builder sets unavailable
    });
    const cell = readSingleCell(col);
    expect(cell.t).toBe("s");
    expect(cell.v).toBe("n/a");
});

⚠️ Low — ### Fixed placed before ### Added in CHANGELOG

File: packages/pluggableWidgets/datagrid-web/CHANGELOG.md
Note: Keep a Changelog specifies section order as Added → Changed → Deprecated → Removed → Fixed → Security. The new entries put ### Fixed above the existing ### Added block. Not blocking, but worth reordering for consistency.

Positives

• Clean extraction of readers into a dedicated cell-readers.ts module — pure functions, no side-effects, easy to unit-test in isolation.
• Overloaded excelDate signatures enforce at the type level that a Date value requires a format argument, preventing accidental date-as-string cells.
• countSignificantDigits correctly handles negatives, decimals, and leading zeros via progressive .replace chaining rather than fragile regex.
• The value !== "" guard before Number(value) in customContent prevents the known Number("") === 0 false positive — the whitespace edge case is the only remaining gap.
• 28 tests cover all three reader paths plus edge cases (loading, unavailable, precision overflow, time-stripping, fallback on parse failure) — thorough coverage for a bug-fix PR.